Skip to content

Stabilize skill loader test isolation#1161

Open
stamsam wants to merge 2 commits into
Gitlawb:mainfrom
stamsam:codex/stabilize-skill-loader-test
Open

Stabilize skill loader test isolation#1161
stamsam wants to merge 2 commits into
Gitlawb:mainfrom
stamsam:codex/stabilize-skill-loader-test

Conversation

@stamsam
Copy link
Copy Markdown

@stamsam stamsam commented May 14, 2026

Summary

  • Narrow the prompt-skill assertion in loadSkillsDir.test.ts to the temp fixture skills root.
  • Prevent globally/user-installed prompt skills from leaking into this test and making the expected skill-name list flaky.

Validation

  • bun test src/skills/loadSkillsDir.test.ts

Notes

This is a tiny test-only cleanup from a fork. No runtime behavior changes.

@stamsam stamsam marked this pull request as ready for review May 14, 2026 03:38
jatmn
jatmn requested changes May 15, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@jatmn jatmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [P2] Keep the isolated skill filter type-safe
    src/skills/loadSkillsDir.test.ts:37
    The new predicate dereferences optional skillRoot, and the combined predicate no longer leaves promptSkills narrowed to prompt commands for the later skillRoot assertions. bun x tsc --noEmit --pretty false now reports errors in this touched test file for skill.skillRoot possibly being undefined and for skillRoot not existing on non-prompt commands. Please guard skillRoot and keep the filtered array narrowed to prompt commands, for example with an explicit type predicate plus optional chaining.

@Vasanthdev2004
Copy link
Copy Markdown
Collaborator

Vasanthdev2004 commented May 16, 2026

Blockers

  1. Type safety issue — The new predicate dereferences optional skillRoot without guarding, and the combined predicate doesn't leave promptSkills narrowed to prompt commands. This causes TypeScript errors in the touched test file.

Non-Blocking

  • Tiny test-only cleanup (5 additions, 1 deletion).

Looks Good

  • Addresses real test flakiness issue
  • Prevents globally/user-installed prompt skills from leaking into test
  • No runtime behavior changes

Verdict: Changes Requested — type safety issue needs to be fixed.

@gnanam1990
Copy link
Copy Markdown
Collaborator

gnanam1990 commented May 17, 2026

Confirmed @jatmn's point. The new isolated-skill predicate dereferences optional skillRoot and the combined predicate no longer narrows the array to prompt commands, so bun x tsc --noEmit reports the touched test file for skill.skillRoot possibly undefined and skillRoot missing on non-prompt commands. An explicit type-predicate that narrows to prompt commands plus optional chaining on skillRoot keeps it type-safe. Small change — happy to re-review as soon as it's green.

@stamsam
Copy link
Copy Markdown
Author

stamsam commented May 17, 2026

Updated the filter to guard optional skillRoot and added an explicit prompt-command type predicate. Focused test passes locally: bun test src/skills/loadSkillsDir.test.ts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jatmn jatmn jatmn requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stamsam @Vasanthdev2004 @gnanam1990 @jatmn